Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.04%. Comparing base (80c2e8e) to head (126df36).

@@           Coverage Diff           @@
##             main   #10894   +/-   ##
=======================================
  Coverage   96.04%   96.04%           
=======================================
  Files         177      177           
  Lines       19627    19630    +3     
=======================================
+ Hits        18850    18853    +3     
  Misses        777      777           

I'm wondering if we should raise if there's both a node and a filepath/module provided. As the result is going to be non sensical if we use the node lineno/end lineno on the other filepath. Maybe what we actually need is a list of "location descriptor" ?)

Brainstorming here, we don't want to break the add_message API it would be hell. So what about something like this:

  @dataclass                                                                                                                                                                                                               
  class MessageLocation:                                    
      module: str                                                                                                                                                                                                          
      filepath: str                                         
      line: int
      col_offset: int = 0
      end_lineno: int | None = None
      end_col_offset: int | None = None

  def add_message(
      self,
      msgid: str,
      line: int | None = None,
      node: NodeNG | None = None,
      args: Any = None,
      confidence: Confidence | None = None,
      col_offset: int | None = None,
      end_lineno: int | None = None,
      end_col_offset: int | None = None,
      additional_locations: list[NodeNG | MessageLocation] | None = None,  # NEW
  ) -> None:

  Then:
  
  self.add_message(
      "R0401",
      line=5,
      module="a_module",
      filepath="/path/a.py",
      args=("a -> b -> c -> a",),
      additional_locations=[
          import_node_in_b,                                    # NodeNG
          MessageLocation("c_module", "/path/c.py", line=3),   # explicit
      ],
  )

  Duplicate code across 3 files:
  self.add_message(
      "R0801",
      line=10,
      module="module_a",
      filepath="/path/a.py",
      args=(3, duplicate_text),
      additional_locations=[
          MessageLocation("module_b", "/path/b.py", line=20, end_lineno=30),
          MessageLocation("module_c", "/path/c.py", line=5, end_lineno=15),
      ],
  )

Do you think we really need the list? For me it would work if we just do node and module + filepath, with an exception if both are provided.

We can enforce that with overloads to better document the API.

If we raise message on multiple files we kinda need it. Right now we raise in one place with a bunch of text that talk about the other node/place. In text that the user has to read it make sense but in Json, sarif or for display in github interface a message for each location make a lot of sense.. i think we also need to have filepath/module for the existing node / the first location in any case as we'll never remove the node to replace by a list. (?) Now that I'm saying it making the node a 'Node | list[Node| message location]' would be incredible, except for the name that would'nt match.

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 126df36

Having more messages depending on the output type doesn't sound like an API we should add. The current API makes sens to me but perhaps @jacobtylerwalls can weigh in?

I think Jacob is right. I explored if there were legitimate use case of giving a node then overriding the node's value. Seems like it's actually used in useless-else-on-loop (the else node doesn't exist in astroid see #10899), but most of the time it's an error that makes the info given worse at the cost of checking each time we add a message if something is overridden or not. Maybe we need to add two "optimized" function add_message_at_node and add_message_at_location and the calling code handle the complexity of raising multiple message or not. (Or making a location from a node if we then discourage the use of add_message). Adding message is not a bottleneck, but it would save 8 conditional per message raised at the cost of having to be explicit when the node isn't good enough.